Skip to content

HADOOP-17870 Make Http Filesystem allow non-absolute uri based paths.#3338

Merged
steveloughran merged 3 commits intoapache:trunkfrom
yellowflash:fix-relative-urls-http-filesystem
Aug 31, 2021
Merged

HADOOP-17870 Make Http Filesystem allow non-absolute uri based paths.#3338
steveloughran merged 3 commits intoapache:trunkfrom
yellowflash:fix-relative-urls-http-filesystem

Conversation

@yellowflash
Copy link
Contributor

@yellowflash yellowflash commented Aug 26, 2021 •
edited
Loading

Description of PR

This PR makes the Http Filesystem consistent with the other filesystems in how the open behaves. It first qualifies the path before making a URI and subsequently URL out of it.

How was this patch tested?

Has modified the existing unit test to cover this case too.

For code changes:

  • Does the title or this PR starts with the corresponding JIRA issue id (e.g. 'HADOOP-17799. Your PR title ...')?
  • Object storage: have the integration tests been executed and the endpoint declared according to the connector-specific documentation?
  • If adding new dependencies to the code, are these dependencies licensed in a way that is compatible for inclusion under ASF 2.0?
  • If applicable, have you updated the LICENSE, LICENSE-binary, NOTICE-binary files?

@steveloughran
Copy link
Contributor

steveloughran commented Aug 26, 2021

LGTM. Are there other calls where we need to call makeQualified?

@yellowflash
Copy link
Contributor Author

yellowflash commented Aug 26, 2021

LGTM. Are there other calls where we need to call makeQualified?

I am not entirely sure. This is something I came across while trying to use ammonite-spark.

@yellowflash yellowflash force-pushed the fix-relative-urls-http-filesystem branch from 3d0e819 to d1b545a Compare August 26, 2021 13:33
ayushtkn
ayushtkn approved these changes Aug 27, 2021
View reviewed changes
Copy link
Member

@ayushtkn ayushtkn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@apache apache deleted a comment from hadoop-yetus Aug 27, 2021
@steveloughran
Copy link
Contributor

steveloughran commented Aug 27, 2021

looks to me like the getFileStatus() call needs the same qualification. Can you add it there + test?

@ayushtkn
Copy link
Member

ayushtkn commented Aug 28, 2021

isn't getFileStatus call not just a dummy? it won't fail right?

  @Override
  public FileStatus getFileStatus(Path path) throws IOException {
    return new FileStatus(-1, false, 1, DEFAULT_BLOCK_SIZE, 0, path);
  }

@steveloughran
Copy link
Contributor

steveloughran commented Aug 28, 2021

think a qualified path should still be in the returned status, as it is effectively what every other FS connector does

@yellowflash yellowflash mentioned this pull request Aug 30, 2021
Closed
@yellowflash
Copy link
Contributor Author

yellowflash commented Aug 30, 2021

think a qualified path should still be in the returned status, as it is effectively what every other FS connector does

Fixed.

@hadoop-yetus
Copy link

hadoop-yetus commented Aug 30, 2021

🎊 +1 overall

Vote Subsystem Runtime Logfile Comment
+0 🆗 reexec 0m 54s Docker mode activated.
_ Prechecks _
+1 💚 dupname 0m 0s No case conflicting files found.
+0 🆗 codespell 0m 0s codespell was not available.
+1 💚 @author 0m 0s The patch does not contain any @author tags.
+1 💚 test4tests 0m 0s The patch appears to include 1 new or modified test files.
_ trunk Compile Tests _
+1 💚 mvninstall 38m 19s trunk passed
+1 💚 compile 22m 49s trunk passed with JDK Ubuntu-11.0.11+9-Ubuntu-0ubuntu2.20.04
+1 💚 compile 19m 39s trunk passed with JDK Private Build-1.8.0_292-8u292-b10-0ubuntu1~20.04-b10
+1 💚 checkstyle 1m 0s trunk passed
+1 💚 mvnsite 1m 31s trunk passed
+1 💚 javadoc 1m 2s trunk passed with JDK Ubuntu-11.0.11+9-Ubuntu-0ubuntu2.20.04
+1 💚 javadoc 1m 35s trunk passed with JDK Private Build-1.8.0_292-8u292-b10-0ubuntu1~20.04-b10
+1 💚 spotbugs 2m 24s trunk passed
+1 💚 shadedclient 18m 8s branch has no errors when building and testing our client artifacts.
_ Patch Compile Tests _
+1 💚 mvninstall 0m 55s the patch passed
+1 💚 compile 22m 1s the patch passed with JDK Ubuntu-11.0.11+9-Ubuntu-0ubuntu2.20.04
+1 💚 javac 22m 1s the patch passed
+1 💚 compile 19m 21s the patch passed with JDK Private Build-1.8.0_292-8u292-b10-0ubuntu1~20.04-b10
+1 💚 javac 19m 21s the patch passed
+1 💚 blanks 0m 0s The patch has no blanks issues.
+1 💚 checkstyle 0m 58s the patch passed
+1 💚 mvnsite 1m 29s the patch passed
+1 💚 javadoc 1m 0s the patch passed with JDK Ubuntu-11.0.11+9-Ubuntu-0ubuntu2.20.04
+1 💚 javadoc 1m 32s the patch passed with JDK Private Build-1.8.0_292-8u292-b10-0ubuntu1~20.04-b10
+1 💚 spotbugs 2m 37s the patch passed
+1 💚 shadedclient 18m 48s patch has no errors when building and testing our client artifacts.
_ Other Tests _
+1 💚 unit 16m 52s hadoop-common in the patch passed.
+1 💚 asflicense 0m 49s The patch does not generate ASF License warnings.
193m 37s
Subsystem Report/Notes
Docker ClientAPI=1.41 ServerAPI=1.41 base: https://ci-hadoop.apache.org/job/hadoop-multibranch/job/PR-3338/4/artifact/out/Dockerfile
GITHUB PR #3338
Optional Tests dupname asflicense compile javac javadoc mvninstall mvnsite unit shadedclient spotbugs checkstyle codespell
uname Linux 00d78ea8d8b5 4.15.0-143-generic #147-Ubuntu SMP Wed Apr 14 16:10:11 UTC 2021 x86_64 x86_64 x86_64 GNU/Linux
Build tool maven
Personality dev-support/bin/hadoop.sh
git revision trunk / 3c06191
Default Java Private Build-1.8.0_292-8u292-b10-0ubuntu1~20.04-b10
Multi-JDK versions /usr/lib/jvm/java-11-openjdk-amd64:Ubuntu-11.0.11+9-Ubuntu-0ubuntu2.20.04 /usr/lib/jvm/java-8-openjdk-amd64:Private Build-1.8.0_292-8u292-b10-0ubuntu1~20.04-b10
Test Results https://ci-hadoop.apache.org/job/hadoop-multibranch/job/PR-3338/4/testReport/
Max. process+thread count 3143 (vs. ulimit of 5500)
modules C: hadoop-common-project/hadoop-common U: hadoop-common-project/hadoop-common
Console output https://ci-hadoop.apache.org/job/hadoop-multibranch/job/PR-3338/4/console
versions git=2.25.1 maven=3.6.3 spotbugs=4.2.2
Powered by Apache Yetus 0.14.0-SNAPSHOT https://yetus.apache.org

This message was automatically generated.

@apache apache deleted a comment from hadoop-yetus Aug 30, 2021
@apache apache deleted a comment from hadoop-yetus Aug 30, 2021
steveloughran
steveloughran approved these changes Aug 30, 2021
View reviewed changes
Copy link
Contributor

@steveloughran steveloughran left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1

merging to trunk and 3.3.x

@steveloughran
Copy link
Contributor

steveloughran commented Aug 30, 2021

@yellowflash -before I merge, how do you want to be credited.?

do you want:

Contributed by yellowflash

Or is there any other name you'd like to use.

@yellowflash
Copy link
Contributor Author

yellowflash commented Aug 30, 2021

@yellowflash -before I merge, how do you want to be credited.?

do you want:

Contributed by yellowflash

Or is there any other name you'd like to use.

Yellowflash is cool :)

@steveloughran steveloughran merged commit 4ea60b5 into apache:trunk Aug 31, 2021
asfgit pushed a commit that referenced this pull request Sep 7, 2021
@yellowflash @steveloughran
HADOOP-17870. Http Filesystem to qualify relative paths. (#3338)
09e8e5c 
Contributed by Yellowflash

Change-Id: I217da06a1a2e5c0ca2b324f8e21baa0846f64858
kiran-maturi pushed a commit to kiran-maturi/hadoop that referenced this pull request Nov 24, 2021
@yellowflash
HADOOP-17870. Http Filesystem to qualify relative paths. (apache#3338)
bcb01d4 
Contributed by Yellowflash
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@steveloughran steveloughran steveloughran approved these changes

@ayushtkn ayushtkn ayushtkn approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@yellowflash @steveloughran @ayushtkn @hadoop-yetus